Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add rustls-platform-verifier flag for importing ConfigBuilderExt #276

Merged
merged 1 commit into from
Jun 7, 2024
Merged

fix: add rustls-platform-verifier flag for importing ConfigBuilderExt #276

merged 1 commit into from
Jun 7, 2024

Conversation

junkurihara
Copy link
Contributor

@junkurihara junkurihara commented Jun 7, 2024 •
edited
Loading

Hello! Thank you so much for actively developing!

I found a (maybe) potential bug when I import hyper-rustls with rustls-platform-verifier feature. If hyper-rustls with rustls-platform-verifier (+aws-lc-rs or ring) and default-features = false, we cannot build the following code

let client =  hyper_rustls::HttpsConnectorBuilder::new().with_platform_verifier()

with error message: "no method named with_platform_verifier found for struct ConfigBuilder in the current scope".

The reason why this build failure occurs is that in src/connector/builder.rs, ConfigBuilderExt is imported only when either one of rustls-native-certs or webpki-roots is enabled. Thus, I fixed the code a bit (just added feature flag).

Of course I know that feature rustls-platform-verifier is not public on README.md and that native-tokio, i.e., rustls-native-certs, seems to be prioritized over the feature. But I am very happy if this would be considered! Thank you.

Jun

djc
djc approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just an oversight, thanks!

@djc djc added this pull request to the merge queue Jun 7, 2024
@djc
Copy link
Member

djc commented Jun 7, 2024

Of course I know that feature rustls-platform-verifier is not public on README.md and that native-tokio, i.e., rustls-native-certs, seems to be prioritized over the feature. But I am very happy if this would be considered! Thank you.

Those are just oversights, too -- happy if you want to send another PR for adding it in README.md. In a future semver-incompatible release I think we'll want to make rustls-platform-verifier the default for clients.

Merged via the queue into rustls:main with commit c56c76a Jun 7, 2024
11 checks passed
@junkurihara
Copy link
Contributor Author

Hi, thank you for the approval!

Those are just oversights, too -- happy if you want to send another PR for adding it in README.md.

Yes, I will make a PR to add rustls-platform-verifier feature in README.md. after getting merged this PR :)

I n a future semver-incompatible release I think we'll want to make rustls-platform-verifier the default for clients.

Yes please! In my opinion, a dedicated feature name is nicer to enable rustls-platform-verifier, e.g., platform-tokio, much like the mapping native-tokio -> rustls-native-certs.

Thanks!

@junkurihara junkurihara mentioned this pull request Jun 7, 2024
Merged
@junkurihara junkurihara deleted the fix/builder-feature-platform-verifier branch June 7, 2024 08:53
@cpu
Copy link
Member

cpu commented Jun 7, 2024

Good catch, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@junkurihara @djc @cpu